-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: handle nested git repos gracefully in checkpoints #8863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Instead of blocking checkpoints when nested git repos are detected, now excludes them from staging to prevent submodule-related errors. Changes: - Removed initialization-time nested repo check that blocked feature - Added findNestedRepos() to detect repos from multiple sources: * .gitmodules (declared submodules) * git index (gitlinks with mode 160000) * filesystem (.git directories and worktrees) - Modified stageAll() to exclude nested repos using pathspec - Added safety check to prevent submodule changes in staging - Updated tests to verify nested repos are excluded from checkpoints This allows checkpoints to work in monorepos and projects with submodules while preventing git errors from nested repo changes.
Reviewed commit c98980d. All previously flagged issues have been resolved.
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review complete. Found 3 issues that should be addressed before approval. Please see the inline comments for details.
Improvements based on reviewer bot feedback: 1. Enhanced safety check (HIGH priority): - Now uses 'git ls-files -s --cached' to detect mode 160000 entries - Catches newly added/removed gitlinks, not just pointer changes - Added check for .gitmodules changes with warning log 2. Improved pathspec exclusion (MEDIUM priority): - Added 'git rm --cached' to remove existing gitlinks before staging - Prevents staging of gitlink entries already in the index - Ensures complete exclusion of nested repo changes 3. Better error logging (LOW priority): - Added warning logs for git command failures in findNestedRepos() - Helps debugging while avoiding feature breakage - Distinguishes expected errors (no .gitmodules) from real issues All tests continue to pass with these enhancements.
|
Posted 2 inline review comments focusing on correctness across platforms and config resolution. Both are minor and straightforward to address.
Mention @roomote in a comment to trigger your PR Fixer agent and make changes to this pull request. |
|
@daniel-lxs do you buy the Oroocle's comments? |
|
Moving back to Changes Requested |
…gitmodules absolute path; add tests; verify no gitlinks staged and log .gitmodules changes
Fixes #8433
Summary
Instead of blocking the checkpoints feature when nested git repositories are detected, this PR modifies the behavior to exclude nested repos from staging, allowing checkpoints to work in monorepos and projects with submodules.
Changes
Benefits
Testing
All existing tests pass, including the updated test that now verifies:
Important
Modifies checkpoints to exclude nested git repos from staging, allowing functionality in monorepos and submodules.
ShadowCheckpointService.ts.stageAll().findNestedRepos()detects nested repos from.gitmodules, git index, and filesystem.ShadowCheckpointService.spec.tsto verify nested repos are excluded..gitmodules, path normalization, and gitlink entries with spaces.This description was created by
for c98980d. You can customize this summary. It will automatically update as commits are pushed.